-
Notifications
You must be signed in to change notification settings - Fork 2
Add sensitive exposure split query #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Can we rename the |
javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposureHeuristicSource.md
Outdated
Show resolved
Hide resolved
javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposureHeuristicSource.md
Outdated
Show resolved
Hide resolved
javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposureHeuristicSource.ql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of thoughts.
predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet contents) { | ||
// Assume all properties of a logged object are themselves logged. | ||
contents = DataFlow::ContentSet::anyProperty() and | ||
isSink(node) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? What kind of code does this intend to capture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its from the out of the box queries, and is very necessary, it means that the definition of a variable being "read" includes any access of its properties (I think at sink locations only, as opposed to overall field sensitivity as an additional flow step would capture)
I dont really see any other form of documentation anywhere talking about this common predicate/to add to the comment but for our own understanding, if you want, searching for "allowImplicitRead" in slack across all channels does find some refs to it and a bit of useful conversation confirming that this is the intended use of that
javascript/frameworks/cap/src/sensitive-exposure/SensitiveExposureHeuristicSource.md
Outdated
Show resolved
Hide resolved
...es/sensitive-exposure/sensitive-exposure-js-all-sinks/sensitive-exposure-heuristic-source.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a second round of review.
Also, please add a description of the relationship of this query to the existing |
What This PR Contributes
js/cap-sensitive-log
query (to avoid duplications) but does use the CAP specific sinks and therefore also avoids duplication of alerts with the out of the box query.Future Works
none at this time